feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

staheri14 · 2023-07-31T12:22:36Z

Overview

Closes ##2138
This PR also addresses the concerns raised in the comment by introducing a distinction between two types of reserved namespaces. Namespaces within the range [0-255] are now identified as "Primary Reserved Namespaces," while any other reserved namespaces beyond this range are categorized as "Non-Primary Reserved Namespaces." Hopefully, this differentiation provides better clarity and organization regarding namespace allocation and usage within the code.

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

…ace' into sanaz/extends-is-reserved-namespace

rootulp

LGTM, I like the addition of "primary". Should we also update it in the specs?

celestia-app/specs/src/specs/namespace.md

Line 77 in 78e5327

    
           | `MAX_RESERVED_NAMESPACE`            | `Namespace` | `0x00000000000000000000000000000000000000000000000000000000FF` | Max reserved namespace is lexicographically the largest namespace that is reserved for protocol use. |

pkg/namespace/namespace.go

x/blob/types/payforblob_test.go

github-actions · 2023-08-02T07:53:24Z

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-2194/
on branch `gh-pages` at 2023-08-03 08:18 UTC

staheri14 · 2023-08-02T08:01:46Z

LGTM, I like the addition of "primary". Should we also update it in the specs?

Good point, please see the updates on the specs in bbffdf3 and 7c76ff6.

evan-forbes

lgtm

cmwaters

I like this change

cmwaters · 2023-08-02T10:58:40Z

specs/src/specs/namespace.md

-In addition to the items listed in this table, it should be noted that namespaces with values less than `0x00000000000000000000000000000000000000000000000000000000FF` are exclusively reserved for use within the Celestia protocols.
+Reserved namespaces fall into two categories, _Primary Reserved Namespaces_ and _Secondary Reserved Namespaces_.
+- Primary Reserved Namespaces: Namespaces with values less than or equal to `0x00000000000000000000000000000000000000000000000000000000FF` are referred to as Primary Reserved Namespace and are exclusively reserved for use within the Celestia protocols.
+- Secondary Reserved Namespaces: Currently, there are two secondary reserved namespaces, namely,  `PARITY_SHARE_NAMESPACE` and `TAIL_PADDING_NAMESPACE` which fall out of the range of primary reserved namespaces.


Do we ever want to give ourselves some padding so we can add secondary namespaces in the future? The same we do with primary?

cmwaters · 2023-08-02T10:59:48Z

x/blob/types/payforblob.go

-	}
-
-	if ns.IsParityShares() {
-		return ErrParitySharesNamespace


Are these errors used anywhere else? Do we want to remove them if they aren't used anywhere?

After integrating this PR, there will be no active usage for ErrParitySharesNamespace in the app. I also initially thought of deleting it, however, there are other unused errors among the existing registered errors e.g., ErrInvalidShareCommitments, so I decided to keep it as is. @rootulp Do you think there will be future usage for this error?

I don't expect future usages of ErrParitySharesNamespace so it seems feasible to remove but we can def do in a follow-up issue / PR.

One question I have is: when we delete errors that are no longer used, do we need to reserve the error code indefinitely or can it be repurposed?

On a related note, sometimes some errors are not used but kept so that their code number is not replaced by some other error which might break downstream for people depending on it

Edit: Rootul front ran me :D

I'm not too sure who uses the error code since the message is always accompanied with it in the response but I'd tend to agree that safest thing to do would be to reserve it (just by leaving a comment probably)

rootulp

I really like the specs change! Thanks for pushing this through

specs/src/specs/namespace.md

rach-id · 2023-08-02T13:48:40Z

x/blob/types/payforblob.go

-	}
-
-	if ns.IsParityShares() {
-		return ErrParitySharesNamespace


On a related note, sometimes some errors are not used but kept so that their code number is not replaced by some other error which might break downstream for people depending on it

Edit: Rootul front ran me :D

…ace' into sanaz/extends-is-reserved-namespace

codecov-commenter · 2023-08-03T08:19:53Z

Codecov Report

Merging #2194 (c0cf2cd) into main (9ea27d4) will decrease coverage by 0.03%.
The diff coverage is 20.00%.

❗ Current head c0cf2cd differs from pull request most recent head b9fdc3c. Consider uploading reports for the commit b9fdc3c to get more accurate results

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
- Coverage   24.89%   24.86%   -0.03%     
==========================================
  Files         127      127              
  Lines       14335    14326       -9     
==========================================
- Hits         3568     3562       -6     
+ Misses      10400    10397       -3     
  Partials      367      367

Files Changed	Coverage Δ
pkg/namespace/namespace.go	`64.48% <0.00%> (-1.87%)`	⬇️
pkg/namespace/random_blob.go	`0.00% <ø> (ø)`
x/blob/types/payforblob.go	`75.70% <100.00%> (-0.67%)`	⬇️

…to IsReserved function (#2194) Closes ##2138 This PR also addresses the concerns raised in the [comment](#2138 (comment)) by introducing a distinction between two types of reserved namespaces. Namespaces within the range [0-255] are now identified as "Primary Reserved Namespaces," while any other reserved namespaces beyond this range are categorized as "Non-Primary Reserved Namespaces." Hopefully, this differentiation provides better clarity and organization regarding namespace allocation and usage within the code. - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords

…to IsReserved function (#2194) ## Overview Closes ##2138 This PR also addresses the concerns raised in the [comment](#2138 (comment)) by introducing a distinction between two types of reserved namespaces. Namespaces within the range [0-255] are now identified as "Primary Reserved Namespaces," while any other reserved namespaces beyond this range are categorized as "Non-Primary Reserved Namespaces." Hopefully, this differentiation provides better clarity and organization regarding namespace allocation and usage within the code. ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords (cherry picked from commit b3b8611) # Conflicts: # pkg/namespace/consts.go # pkg/namespace/namespace.go # specs/src/specs/namespace.md

staheri14 and others added 4 commits July 31, 2023 15:02

includes parity namespace and tail padding namespace in the IsReserved

89b0b20

deletes unnecessary calls to IsParityShare and IsTailPadding

3ff0262

changes a var name

6a3c1c2

Merge branch 'main' into sanaz/extends-is-reserved-namespace

3ca80a0

staheri14 self-assigned this Aug 1, 2023

staheri14 changed the title ~~feat: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function~~ feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function Aug 1, 2023

staheri14 added this to the Mainnet milestone Aug 1, 2023

staheri14 added 4 commits August 1, 2023 17:23

renames MaxReservedNamespace to MaxPrimaryReservedNamespace

0da6cb3

Merge remote-tracking branch 'origin/sanaz/extends-is-reserved-namesp…

81e72da

…ace' into sanaz/extends-is-reserved-namespace

edits the const description

ad875d7

renames a var

4e8d911

staheri14 marked this pull request as ready for review August 1, 2023 14:42

staheri14 requested review from evan-forbes, rootulp and cmwaters as code owners August 1, 2023 14:42

staheri14 added warn:api breaking item will be break an API and require a major bump and removed warn:api breaking item will be break an API and require a major bump labels Aug 1, 2023

staheri14 linked an issue Aug 1, 2023 that may be closed by this pull request

Incorporate ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2138

Closed

staheri14 enabled auto-merge (squash) August 1, 2023 15:01

rootulp previously approved these changes Aug 1, 2023

View reviewed changes

pkg/namespace/namespace.go Outdated Show resolved Hide resolved

x/blob/types/payforblob_test.go Outdated Show resolved Hide resolved

staheri14 added 2 commits August 2, 2023 10:52

addresses comments

1768e62

updates specs

bbffdf3

staheri14 dismissed rootulp’s stale review via bbffdf3 August 2, 2023 07:53

staheri14 requested review from liamsi and adlerjohn as code owners August 2, 2023 07:53

MSevey requested a review from a team August 2, 2023 07:53

updates the table

7c76ff6

staheri14 force-pushed the sanaz/extends-is-reserved-namespace branch from d09f934 to 7c76ff6 Compare August 2, 2023 08:01

staheri14 requested a review from rootulp August 2, 2023 08:03

evan-forbes previously approved these changes Aug 2, 2023

View reviewed changes

cmwaters previously approved these changes Aug 2, 2023

View reviewed changes

Merge branch 'main' into sanaz/extends-is-reserved-namespace

d3b29c8

rootulp previously approved these changes Aug 2, 2023

View reviewed changes

rach-id reviewed Aug 2, 2023

View reviewed changes

rootulp dismissed stale reviews from cmwaters, evan-forbes, and themself via d3b29c8 August 2, 2023 14:32

MSevey requested a review from a team August 2, 2023 14:33

staheri14 and others added 3 commits August 3, 2023 11:11

Merge branch 'main' into sanaz/extends-is-reserved-namespace

c0cf2cd

addresses editorial comments

4658131

Merge remote-tracking branch 'origin/sanaz/extends-is-reserved-namesp…

b9fdc3c

…ace' into sanaz/extends-is-reserved-namespace

staheri14 requested review from rootulp, rach-id, cmwaters and evan-forbes August 3, 2023 08:19

evan-forbes approved these changes Aug 3, 2023

View reviewed changes

rootulp approved these changes Aug 3, 2023

View reviewed changes

staheri14 merged commit b3b8611 into main Aug 3, 2023
27 checks passed

staheri14 deleted the sanaz/extends-is-reserved-namespace branch August 3, 2023 13:12

evan-forbes added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Sep 5, 2023

mergify bot mentioned this pull request Sep 5, 2023

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function (backport #2194) #2422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

staheri14 commented Jul 31, 2023 •

edited

Loading

rootulp left a comment

github-actions bot commented Aug 2, 2023 •

edited

Loading

staheri14 commented Aug 2, 2023

evan-forbes left a comment

cmwaters left a comment

cmwaters Aug 2, 2023

cmwaters Aug 2, 2023

staheri14 Aug 2, 2023

rootulp Aug 2, 2023

rach-id Aug 2, 2023 •

edited

Loading

cmwaters Aug 2, 2023

rootulp left a comment

rach-id Aug 2, 2023 •

edited

Loading

codecov-commenter commented Aug 3, 2023

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

feat!: incorporates ParitySharesNamespace and TailPaddingNamespace into IsReserved function #2194

Conversation

staheri14 commented Jul 31, 2023 • edited Loading

Overview

Checklist

rootulp left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 2, 2023 • edited Loading

staheri14 commented Aug 2, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

cmwaters Aug 2, 2023

Choose a reason for hiding this comment

cmwaters Aug 2, 2023

Choose a reason for hiding this comment

staheri14 Aug 2, 2023

Choose a reason for hiding this comment

rootulp Aug 2, 2023

Choose a reason for hiding this comment

rach-id Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

cmwaters Aug 2, 2023

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rach-id Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Aug 3, 2023

Codecov Report

staheri14 commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

rach-id Aug 2, 2023 •

edited

Loading

rach-id Aug 2, 2023 •

edited

Loading